Replace relative imports by absolute ones in categories #36572

tobiasdiez · 2023-10-29T16:41:32Z

As preparation for compiling with meson, we replace relative imports by absolute ones.
Relative imports are not used consistently in the codebase and result in issues for doctesting with pytest (which admittedly is a limitation of pytest). We normalize the relative imports in sage.categories to be absolute imports. Small code-style improvements along the way (mainly to nicely order the imports)

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

mkoeppe · 2023-10-29T20:27:07Z

You may be interested to know that I have these changes in #35095.

See also #36228

Edit: For reference, commits dfceb98, 7d04241, ef2f651 make these changes in #35095.

kwankyu · 2023-11-01T20:43:42Z

Is this just to satisfy buggy pytest?

So are you recommending no relative imports in our codebase?

tobiasdiez · 2023-11-02T00:42:22Z

Is this just to satisfy buggy pytest?

And buggy cython (which also has problems with relative imports without __init__ files)

So are you recommending no relative imports in our codebase?

Yes.

mkoeppe · 2023-11-03T17:22:46Z

Same comment as #36588 (comment)

mkoeppe · 2023-11-07T16:10:43Z

Rebased cleanly -- without need for manual intervention -- on top of #36666.

(Edit: see #36666 (comment))

tobiasdiez · 2023-11-07T16:30:13Z

Please revert these changes (and never force push to my PRs again). Thanks.

tobiasdiez · 2023-11-09T01:05:38Z

@mkoeppe please revert the changes you made to this PR. Thanks

mkoeppe · 2023-11-09T01:58:30Z

Please revert these changes (and never force push to my PRs again).

Tobias, I know that you know that but I'll explain it to the audience:

This PR is marked "Maintainers are allowed to edit this pull request." If you wish your PRs to not undergo normal maintainer actions -- such as constructive rebasing, as discussed in #36666 (comment) -- the correct protocol is to just uncheck that checkmark.

And to force-push whatever branch you may have -- the old one or an updated one -- you do not need my assistance to push it to your PR.

This type of aggressive posturing ("never ... again") is most unwelcome here.

More generally: Your frequent insinuations that my work on PRs is somehow "improper" are also unwelcome conduct; the frequent repetition makes it harassment. The sage-abuse committee has already informed you recently in #36489 (comment) about the correct procedure:

"If you feel that anybody else's behavior violates the code of conduct, we encourage you to email [2] and explain why. [2] https://groups.google.com/g/sage-abuse"

tobiasdiez · 2023-11-09T04:51:12Z

Your force push results in problems in my normal workflow (and it's generally advised to not force push to any public branch that is shared with other people). Moreover, I don't have a local copy of the original branch since I'm exclusively using codespaces. So yes, I would like you to restore the branch.

mkoeppe · 2023-11-09T19:07:31Z

it's generally advised to not force push to any public branch that is shared with other people

I know that you know that, but I'll explain it to the audience: Rebasing PR branches is very much standard procedure on GitHub. The general advice to not force push to public branches refers to branches that are referred to by branch name.

mkoeppe · 2023-11-09T19:09:22Z

Your force push results in problems in my normal workflow

If you need help with rebasing a branch, just ask for help.

tobiasdiez · 2023-11-09T20:28:34Z

it's generally advised to not force push to any public branch that is shared with other people

I know that you know that, but I'll explain it to the audience: Rebasing PR branches is very much standard procedure on GitHub. The general advice to not force push to public branches refers to branches that are referred to by branch name.

Let me update your knowledge by citing the github and gitlab documentation:

Be very careful when force pushing commits to a pull request. Force pushing changes the repository history and can corrupt your pull request. If other collaborators branch the project before a force push, the force push may overwrite commits that collaborators based their work on.

(https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests)

git rebase rewrites the commit history. It can be harmful to do it in shared branches. It can cause complex and hard to resolve merge conflicts. In these cases, instead of rebasing your branch against the default branch, consider pulling it instead (git pull origin master). Pulling has similar effects with less risk compromising the work of your contributors.

(https://docs.gitlab.com/ee/topics/git/git_rebase.html)

But in either case, please respect my workflow.

mkoeppe · 2023-11-09T20:52:07Z

by citing the github and gitlab documentation:

Be very careful when force pushing commits to a pull request.

Yes, one has to be careful.

tobiasdiez · 2023-11-09T21:09:35Z

Matthias, please just revert your force push. Thanks

…ories

github-actions · 2023-11-26T15:45:28Z

Documentation preview for this PR (built with commit df2f18d; changes) is ready! 🎉

dimpase

lgtm

tobiasdiez · 2023-11-27T13:56:01Z

Thanks!

…ries      As preparation for compiling with meson, we replace relative imports by absolute ones. Relative imports are not used consistently in the codebase and result in issues for doctesting with pytest (which admittedly is a limitation of pytest). We normalize the relative imports in `sage.categories` to be absolute imports. Small code-style improvements along the way (mainly to nicely order the imports) ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36572 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik

…{algebras,arith,categories,cpython,data_structures,misc,modular,rings,sat,symbolic}`    Final chunk of the cherry-pick from sagemath#35095 for sagemath#36228, see also sagemath#36572 (comment) This PR overlaps with sagemath#36572, sagemath#36588, sagemath#36589, but it does not touch `.all*` files (no changes there are needed for sagemath#36228), thus avoiding conflicts with sagemath#35095 (see also sagemath#36524 (comment))   ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36666 Reported by: Matthias Köppe Reviewer(s): David Coudert, John H. Palmieri

…e relative by absolute imports   We restructure the `all.py` files for modularization. Guided by the technical constraints outlined in https://groups.google.com/g/sage-devel/c/ozh-7ZZ848s, sagemath#35095 defines distribution packages (pip-installable packages) **sagemath-{brial, combinat, eclib, flint, gap, giac, glpk, graphs, groups, homfly, lcalc, libbraiding, libecm, linbox, modules, mpmath, ntl, pari, plot, polyhedra, schemes, singular, standard-no-symbolics, symbolics}**. When a namespace package such as `sage.misc` is filled by modules from several distribution packages, we create modules named: - `src/sage/misc/all__sagemath_environment.py` - `src/sage/misc/all__sagemath_objects.py` - `src/sage/misc/all__sagemath_repl.py` Import statements are moved from `src/sage/misc/all.py` to these files as appropriate, and `src/sage/misc/all.py` imports `*` from there. Also some imports are replaced by lazy imports. The new files provide the top level namespaces for the modularized distribution packages, thus [enabling modularized testing](https://doc.s agemath.org/html/en/developer/packaging_sage_library.html#testing- distribution-packages). This design was introduced in sagemath#29865 (merged in Jan 2022, early in the Sage 9.6 development cycle).   - Copied from sagemath#35095. - Part of sagemath#29705 Moreover, applied a one-line command to replace relative by absolute imports, thus complementing sagemath#36666, which does not touch `.all*` files. The changes to other files in `sage.modules` etc. come from the PRs sagemath#36597, sagemath#36572, sagemath#36588, sagemath#36589 merged here and do not need review.  ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies  - Depends on sagemath#36597 (merged here to resolve merge conflict) - Depends on sagemath#36572 (merged here to resolve merge conflict) - Depends on sagemath#36588 (merged here to resolve merge conflict) - Depends on sagemath#36589 (merged here to resolve merge conflict) - Depends on sagemath#36449 (merged here to resolve merge conflict)  URL: sagemath#36676 Reported by: Matthias Köppe Reviewer(s): David Coudert, John H. Palmieri, Matthias Köppe, Tobias Diez

@mkoeppe

Author: @mkoeppe, based on @tobiasdiez's config in sagemath#36503. Adding a configuration for the ruff linter as proposed in sagemath#36503 is timely and uncontroversial. However, sagemath#36503 bundled this gratuitously with the controversial design of creating a `pyproject.toml` file in SAGE_ROOT. `pyproject.toml` -- by definition in PEP 518, PEP 621 -- marks a directory as a Python project, i.e., the source directory of a Python distribution package (sagemath#36503 (comment)). Generalizing the use of `pyproject.toml` to "[non-package projects](https://peps.python.org/pep-0735/#motivation)" is still subject to discussion in the Python community in [PEP 735](https://peps.python.org/pep-0735/) (Nov 2023). **The scope of PRs should be chosen to minimize friction, not to maximize friction.** sagemath#36726 (comment) Here we remove the artificial friction from the gratuitous bundling, by configuring ruff in `ruff.toml` instead. Configuration in ruff.toml and pyproject.toml has equal validity / standing per [ruff documentation](https://docs.astral.sh/ruff/configuration/#config-file- discovery). And this is the location of most of our other linter configuration files. Reference on previous common denominator PRs: sagemath#36666 (comment), sagemath#36666 (comment), sagemath#36572 (comment), sagemath#36960 (comment), sagemath#36960 (comment), ...    ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#37452 Reported by: Matthias Köppe Reviewer(s): Giacomo Pope, Matthias Köppe

…e relative by absolute imports   We restructure the `all.py` files for modularization. Guided by the technical constraints outlined in https://groups.google.com/g/sage-devel/c/ozh-7ZZ848s, sagemath#35095 defines distribution packages (pip-installable packages) **sagemath-{brial, combinat, eclib, flint, gap, giac, glpk, graphs, groups, homfly, lcalc, libbraiding, libecm, linbox, modules, mpmath, ntl, pari, plot, polyhedra, schemes, singular, standard-no-symbolics, symbolics}**. When a namespace package such as `sage.misc` is filled by modules from several distribution packages, we create modules named: - `src/sage/misc/all__sagemath_environment.py` - `src/sage/misc/all__sagemath_objects.py` - `src/sage/misc/all__sagemath_repl.py` Import statements are moved from `src/sage/misc/all.py` to these files as appropriate, and `src/sage/misc/all.py` imports `*` from there. Also some imports are replaced by lazy imports. The new files provide the top level namespaces for the modularized distribution packages, thus [enabling modularized testing](https://doc.s agemath.org/html/en/developer/packaging_sage_library.html#testing- distribution-packages). This design was introduced in sagemath#29865 (merged in Jan 2022, early in the Sage 9.6 development cycle).   - Copied from sagemath#35095. - Part of sagemath#29705 Moreover, applied a one-line command to replace relative by absolute imports, thus complementing sagemath#36666, which does not touch `.all*` files. The changes to other files in `sage.modules` etc. come from the PRs sagemath#36597, sagemath#36572, sagemath#36588, sagemath#36589 merged here and do not need review.  ### 📝 Checklist     - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies  - Depends on sagemath#36597 (merged here to resolve merge conflict) - Depends on sagemath#36572 (merged here to resolve merge conflict) - Depends on sagemath#36588 (merged here to resolve merge conflict) - Depends on sagemath#36589 (merged here to resolve merge conflict) - Depends on sagemath#36449 (merged here to resolve merge conflict)  URL: sagemath#36676 Reported by: Matthias Köppe Reviewer(s): David Coudert, John H. Palmieri, Kwankyu Lee, Matthias Köppe, Tobias Diez

Replace relative imports by absolute ones in categories

9eab458

cleanup

bac9d7c

tobiasdiez marked this pull request as ready for review October 31, 2023 00:11

tobiasdiez requested review from mkoeppe and tornaria October 31, 2023 00:11

tobiasdiez added the s: needs review label Oct 31, 2023

Merge branch 'develop' into rel_import_categories

08ab87a

Merge branch 'develop' into rel_import_categories

bafc803

mkoeppe force-pushed the rel_import_categories branch from bafc803 to 5caaa9e Compare November 7, 2023 16:09

mkoeppe added the t: refactoring label Nov 7, 2023

tobiasdiez mentioned this pull request Nov 11, 2023

Restructure sage.*.all for modularization, replace relative by absolute imports #36676

Closed

5 tasks

vbraun force-pushed the develop branch from 883e05f to e349b00 Compare November 12, 2023 16:25

Merge remote-tracking branch 'upstream/develop' into rel_import_categ…

df2f18d

…ories

tobiasdiez force-pushed the rel_import_categories branch from 5caaa9e to df2f18d Compare November 26, 2023 14:52

tobiasdiez requested a review from dimpase November 26, 2023 14:55

tobiasdiez mentioned this pull request Nov 26, 2023

Replace relative imports by absolute ones in rings #36588

Merged

5 tasks

dimpase approved these changes Nov 27, 2023

View reviewed changes

dimpase added s: positive review and removed s: needs review labels Nov 27, 2023

vbraun merged commit 9014410 into sagemath:develop Dec 6, 2023
20 checks passed

mkoeppe added this to the sage-10.3 milestone Dec 6, 2023

github-actions bot removed the s: positive review label Dec 6, 2023

mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Dec 23, 2023

saraedum mentioned this pull request Apr 29, 2024

Restructure sage.*.all for modularization, replace relative by absolute imports #37900

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace relative imports by absolute ones in categories #36572

Replace relative imports by absolute ones in categories #36572

tobiasdiez commented Oct 29, 2023 •

edited

Loading

mkoeppe commented Oct 29, 2023 •

edited

Loading

kwankyu commented Nov 1, 2023

tobiasdiez commented Nov 2, 2023

mkoeppe commented Nov 3, 2023

mkoeppe commented Nov 7, 2023 •

edited

Loading

tobiasdiez commented Nov 7, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 26, 2023

dimpase left a comment

tobiasdiez commented Nov 27, 2023

Replace relative imports by absolute ones in categories #36572

Replace relative imports by absolute ones in categories #36572

Conversation

tobiasdiez commented Oct 29, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

mkoeppe commented Oct 29, 2023 • edited Loading

kwankyu commented Nov 1, 2023

tobiasdiez commented Nov 2, 2023

mkoeppe commented Nov 3, 2023

mkoeppe commented Nov 7, 2023 • edited Loading

tobiasdiez commented Nov 7, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023 • edited Loading

github-actions bot commented Nov 26, 2023

dimpase left a comment

Choose a reason for hiding this comment

tobiasdiez commented Nov 27, 2023

tobiasdiez commented Oct 29, 2023 •

edited

Loading

mkoeppe commented Oct 29, 2023 •

edited

Loading

mkoeppe commented Nov 7, 2023 •

edited

Loading

tobiasdiez commented Nov 9, 2023 •

edited

Loading